-
Notifications
You must be signed in to change notification settings - Fork 203
BYOC: add streaming #3727
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
BYOC: add streaming #3727
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3727 +/- ##
===================================================
+ Coverage 31.54628% 32.93201% +1.38573%
===================================================
Files 159 160 +1
Lines 38984 40283 +1299
===================================================
+ Hits 12298 13266 +968
- Misses 25797 25979 +182
- Partials 889 1038 +149
... and 3 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
fdc7fc4 to
34e53b9
Compare
| } | ||
| stopJob.sign() //no changes to make, sign job | ||
|
|
||
| token, err := sessionToToken(params.liveParams.sess) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets check/log this err here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still hoping we can check and log this err
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added an error check and log in d73fe37. Note that this function cannot return an error currently, should I just update to only return the JobToken. I think I added an error here thinking as I built it out something could cause an error possibly.
e862a20 to
33fda51
Compare
…able live payments
server/job_rpc.go
Outdated
| balance = node.Balances.Balance(orchAddr, core.ManifestID(jobReq.Capability)) | ||
| } | ||
|
|
||
| diff := new(big.Rat).Sub(orchBal, balance) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
had some observations and questions here ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe out of bounds for this PR, but still maybe some legit concern here:
- What could happen if Orchestrators can influence Gateway accounting?
- Should there be bounds on balance adjustments?
- How do we handle legitimate consumption vs. potential manipulation?
I think this is a good chance to think through payment security patterns here, such as:
- What happens if
orchToken.Balanceis higher thanbalance?
( how does transcoding payment flows handle this? )
And we should probably consider adding validation logic, i.e. bounds
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe out of bounds for this PR, but still maybe some legit concern here:
What could happen if Orchestrators can influence Gateway accounting?
Should there be bounds on balance adjustments?
How do we handle legitimate consumption vs. potential manipulation?
This is a possible threat to Gateways for sure but is also a threat on every other pipeline in Livepeer because the Orchestrator always wins. If an Orchestrator continusously returns no balance then Gateways have to create tickets to pay for it or the Orchestrator theoretically would stop work.
Risk to Orchestrators is the Gateway never uses them again.
What happens if orchToken.Balance is higher than balance?
( how does transcoding payment flows handle this? )
There is no balance reporting back to the Gateway outside of BYOC. For transcoding, all balance is lost when the stream starts since it is tracked on manifest ID and balances are cleared at the end of a stream. Similar path is used for batch AI requests. BYOC takes a different approach using the capability name as the balance key to track the balance so balance can be shared across requests.
I was using this as a first step to start that bi directional communication on balance with the plan to tighten it up after some more real world testing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did some updates and moved the compare/adjust/report in createPayment to a new function of AddressBalances that uses a new mutex to lock. The mutex in address balances is used for each individual operation (Balance/Debit/Credit). New sharedBalMtx mutex wraps all those operations to avoid changes while updating.
Also added a lock around payment application for the Orchestrator monitoring go routine.
server/job_stream.go
Outdated
|
|
||
| //start payment monitoring | ||
| go func() { | ||
| stream, _ := h.node.ExternalCapabilities.Streams[orchJob.Req.ID] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this accesses ExternalCapabilities.Streams map directly without holding the capm mutex. any concurrent map access will throw panics when streams are added/removed unless we acquire the lock first. I think we should follow a pattern similar to how StreamExists() is used - adding a helper GetStream(streamID) could be useful for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just noticed that we're re-using this - we should probably re-capture each time instead of re-using stream
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in e9a4c9d
server/job_stream.go
Outdated
| } | ||
|
|
||
| clog.Infof(ctx, "Insufficient balance, stopping stream %s for sender %s", orchJob.Req.ID, orchJob.Sender) | ||
| _, exists := h.node.ExternalCapabilities.Streams[orchJob.Req.ID] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see previous
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in e9a4c9d
server/job_stream.go
Outdated
|
|
||
| //check if stream still exists | ||
| // if not, send stop to worker and exit monitoring | ||
| stream, exists := h.node.ExternalCapabilities.Streams[orchJob.Req.ID] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here too ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in e9a4c9d
| if !exists { | ||
| clog.Errorf(ctx, "Stream %s not found", streamId) | ||
| return | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
possibly add
defer ls.LivepeerNode.RemoveLivePipeline(streamId)
to ensure cleanup even if something goes wrong
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved RemoveLivePipeline call up to defer statement before the loop. RemoveLivePipeline just deletes from the map and is a noop if already deleted. See f8beda8
| priceInfo := sess.OrchestratorInfo.PriceInfo | ||
| var paymentProcessor *LivePaymentProcessor | ||
| if priceInfo != nil && priceInfo.PricePerUnit != 0 { | ||
| if priceInfo != nil && priceInfo.PricePerUnit != 0 && sess.OrchestratorInfo.AuthToken != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add comment documenting why we're using sess.OrchestratorInfo.AuthToken check here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See 17357af
|
|
||
| //ensure streamRequest is not nil or empty | ||
| if streamRequest == nil || len(streamRequest) == 0 { | ||
| streamRequest = []byte("{}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets make a note here that we either require an explicit empty json object from the caller ( or somehow else valid as a default - you said in chat it was upstream? )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see 421ba70
2b02214 to
db1df66
Compare
d9aac9a to
421ba70
Compare
What does this pull request do? Explain your changes. (required)
Adds configurable streaming for BYOC entrypoint to go-livepeer. Uses trickle protocol to handle streaming for similar entrypoints and outputs from go-livepeer as live-video-to-video.
Streams can be any or a mix of the following:
Control and Events channels are created for every stream.
Streams are created with a POST request to
/ai/stream/startthat will start the stream and reserve the capacity with an Orchestrator that is providing the BYOC capability. If video ingress is enabled, the client should then start a stream with WHIP or RTMP to the provided ingress URLs provided in the response. URLs for egress video, data, updates (control) and events are also included in the response as well as the stream_id. The stream_id is an integral part of the URLs provided to interact with the stream and is combined with a provided stream name in the /ai/stream/start request.Streams are stopped with a POST request to
/ai/stream/stop. Orchestrators and Gateways track payment balance and the Gateway adjusts to the Orchestrators provided balance in new JobTokens provided at each payment interval every minute. Orchestrators will shutdown a stream when payment balance is zero.Specific updates (required)
job_stream.goandjob_stream_test.gojob_rpc.goto reuse stream setup where made sensecommon/testutil.go.How did you test each of these updates (required)
Used
byoc-streamto test end to end: https://github.com/ad-astra-video/livepeer-app-pipelines/tree/main/byoc-streamAdded tests to
job_stream_test.goand some additional tests tojob_rpc_test.go.Does this pull request close any open issues?
Checklist:
makeruns successfully./test.shpass